Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: improve startup and shutdown #1126

Merged
merged 1 commit into from
Oct 2, 2024
Merged

Conversation

ava-fred
Copy link
Contributor

@ava-fred ava-fred commented Oct 1, 2024

In #1103, it was noted that
LanguageServerWrapperTest#testStopAndActivate causes OOM errors. On investigation, it was discovered that there are multiple causes for this.

  1. The MockConnectionProviderMultiRootFolder used in the test created a message processor thread in LSP4J on each call to start() but did not shut them down.
  2. The termination of the start/stop loop in the test was wrong: the loop ran for as long as the VM running tests was alive. 3) The "already stopping" logic in LanguageServerWrapper#shutdown was wrong: while the shutdown of a LanguageServerWrapper was processed, further calls to shutdown were ignored.
  3. There was a race between the initializationFuture of the LanguageServerWrapper and the future used for shutdown.

With this commit, the test is rewritten to avoid crashes and to properly test that calling stop / start repeatedly on LanguageServerWrapper does not leave any connection providers running. The LanguageServerWrapper class is refactored to make the test pass.

In eclipse#1103, it was noted that
LanguageServerWrapperTest#testStopAndActivate causes OOM errors. On
investigation, it was discovered that there are multiple causes for
this.

1) The MockConnectionProviderMultiRootFolder used in the test created a
message processor thread in LSP4J on each call to start() but did not
shut them down.
2) The termination of the start/stop loop in the test was wrong: the
loop ran for as long as the VM running tests was alive.
3) The "already stopping" logic in LanguageServerWrapper#shutdown was
wrong: while the shutdown of a LanguageServerWrapper was processed,
further calls to shutdown were ignored.
4) There was a race between the initializationFuture of the
LanguageServerWrapper and the future used for shutdown.

With this commit, the test is rewritten to avoid crashes and to properly
test that calling stop / start repeatedly on LanguageServerWrapper does
not leave any connection providers running. The LanguageServerWrapper
class is refactored to make the test pass.
@rubenporras rubenporras merged commit ca55022 into eclipse:main Oct 2, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants